fix(evidence): stop EvidenceAccumulator deadlocking on reentrant append/seal - #46
Conversation
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed the accumulator lock and callback handling at this head. All 14 evidence tests pass locally. Independent checks also pass for retrying the same accumulator after rejected callback reentry, 16 concurrent appends with contiguous sequence numbers, and sealing the resulting 17 entries. Restoring the prior implementation makes the reentrant callback hang until the five-second test timeout. CI is green and there are no unresolved review threads.
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed the accumulator lock and callback handling at this head. All 14 evidence tests pass locally. Independent checks also pass for retrying the same accumulator after rejected callback reentry, 16 concurrent appends with contiguous sequence numbers, and sealing the resulting 17 entries. Restoring the prior implementation makes the reentrant callback hang until the five-second test timeout. CI is green and there are no unresolved review threads.
2 participants
Summary
Python's EvidenceAccumulator used a plain threading.Lock (non-reentrant). If a durable_append callback called back into append(), seal(), or snapshot() on the same thread, it deadlocked forever with no error. The TypeScript SDK already has this exact reentrancy protection (added in #21) Python never got it. This adds the same guard: append/seal now raise EvidenceError on reentrant calls instead of hanging; snapshot stays safely callable reentrantly since it only reads committed state.
Contract impact
Evidence
python tools/sync_schemas.pypython tools/check_schemas.pypython conformance/runner/validate.pypython -m unittest discover -s tests -vpython -m buildPrivacy and security
Limits
This PR fixesthe reentrant calls from the same thread (e.g. a callback calling back into the accumulator). It does not add general thread-safety to other classes like UsageAccumulator, it still has no lock and isnt covered here. It also doesn't address Lock+fork() interaction, which is a pre-existing, unrelated limitation.